feat: track previousNextUrl for intercepted App Router entries#755
Conversation
commit: |
NathanDrake2406
changed the title
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-5-previous-next-url
branch
3 times, most recently
from
d11ce67 to
6c5944b
Compare
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-5-previous-next-url
branch
from
6c5944b to
252f3d0
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: track previousNextUrl for intercepted App Router entries
Overall this is a clean, well-structured PR. The separation of previousNextUrl (full URL with search params) from interceptionContext (pathname-only, stripped of basePath) is a good design choice — it preserves the information needed for both history state restoration and RSC request headers without conflating the two. The hard-load normalization at boot (createHistoryStateWithPreviousNextUrl(window.history.state, null)) is the right call for breaking out of interception on reload.
A few items to address:
Correctness concern: stale __vinext_interceptionContext in history state
The old code wrote __vinext_interceptionContext into history.state via createHistoryStateWithInterceptionContext. This PR replaces that with __vinext_previousNextUrl. However, getCurrentInterceptionContext() in navigation.ts:289-298 still reads from __vinext_interceptionContext via readHistoryStateInterceptionContext(window.history.state). Since no code writes that key anymore, readHistoryStateInterceptionContext will always return null for new history entries, and getCurrentInterceptionContext will fall through to stripBasePath(window.location.pathname, __basePath) — which happens to be the correct value for fresh navigations.
But this creates a subtle asymmetry: existing history entries (from before the code update, e.g., in dev during HMR, or across deployments) may still carry the old __vinext_interceptionContext key, while new entries carry __vinext_previousNextUrl. getCurrentInterceptionContext() would read stale __vinext_interceptionContext from old entries. This is probably fine in practice (the old entries would have the correct value), but the dead-code path through readHistoryStateInterceptionContext is now confusing. Consider either:
- Removing
readHistoryStateInterceptionContextfromgetCurrentInterceptionContext()so it always derives from pathname (matching the new single-source-of-truth model), or - Adding a migration note / TODO for the next PR in the stack.
The VINEXT_INTERCEPTION_CONTEXT_HISTORY_STATE_KEY export and readHistoryStateInterceptionContext are now effectively dead code from the browser entry's perspective.
Missing E2E test mentioned in PR description
The PR description says it adds E2E coverage for "back/forward restoration" and references the grep pattern back then forward restores intercepted modal view. However, there is no test with that name in the diff. The diff only adds two E2E tests: hard reload after intercepted navigation renders the full page and router.refresh preserves intercepted modal view. Was the back/forward traversal test accidentally omitted?
Minor: double getBrowserRouterState() call in refresh case
See inline comment.
Minor: resolveInterceptionContextFromPreviousNextUrl basePath default
See inline comment.
| case "refresh": | ||
| return null; | ||
| return { | ||
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | ||
| getBrowserRouterState().previousNextUrl, | ||
| __basePath, | ||
| ), | ||
| previousNextUrl: getBrowserRouterState().previousNextUrl, |
There was a problem hiding this comment.
Nit: getBrowserRouterState() is called twice in the refresh case — once at line 340 and again at line 343. The ref read is cheap, but it would be slightly cleaner to bind once:
| case "refresh": | |
| return null; | |
| return { | |
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | |
| getBrowserRouterState().previousNextUrl, | |
| __basePath, | |
| ), | |
| previousNextUrl: getBrowserRouterState().previousNextUrl, | |
| case "refresh": { | |
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | |
| return { | |
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | |
| currentPreviousNextUrl, | |
| __basePath, | |
| ), | |
| previousNextUrl: currentPreviousNextUrl, | |
| }; | |
| } |
|
|
||
| export function resolveInterceptionContextFromPreviousNextUrl( | ||
| previousNextUrl: string | null, | ||
| basePath: string = "", |
There was a problem hiding this comment.
The basePath parameter defaults to "", which is correct for the no-basePath case, but all callers in app-browser-entry.ts explicitly pass __basePath. Consider whether the default is actually needed or if it would be safer to make it required. As-is, an accidental omission would silently produce the wrong interception context for apps with a basePath.
That said, it's useful for tests (which don't have a basePath). Leaving it as-is is fine — just noting the trade-off.
| }): Promise<PendingNavigationCommit> { | ||
| const elements = await options.nextElements; | ||
| const metadata = readAppElementsMetadata(elements); | ||
| const previousNextUrl = options.previousNextUrl ?? options.currentState.previousNextUrl; |
There was a problem hiding this comment.
This fallback is well-designed — if previousNextUrl isn't explicitly provided, it falls through to currentState.previousNextUrl, which preserves the existing interception provenance through server-action merges. Good.
| createHistoryStateWithPreviousNextUrl(window.history.state, null), | ||
| "", | ||
| window.location.href, | ||
| ); |
There was a problem hiding this comment.
This is the correct place to normalize on hard load. Browsers preserve history.state across reloads, so without this, a reload would carry over the previousNextUrl from the intercepted navigation and the refresh request would re-fetch the intercepted view instead of the full page. Good.
Worth noting: this also implicitly clears any leftover __vinext_interceptionContext from the old history state format (since createHistoryStateWithPreviousNextUrl clones only existing keys and writes the new key). That's a nice side effect, though it depends on the old key not being actively read from history state elsewhere.
| ); | ||
| } | ||
|
|
||
| export function getCurrentNextUrl(): string { |
There was a problem hiding this comment.
This function returns the full pathname + search without stripping basePath. That's intentional — previousNextUrl should preserve the full URL as the user sees it, and resolveInterceptionContextFromPreviousNextUrl handles the basePath stripping separately when deriving the interception context.
However, this means the server sees interception context derived from a URL that includes basePath in previousNextUrl but has it stripped in interceptionContext. I'd expect this to work correctly given the resolveInterceptionContextFromPreviousNextUrl function, but it's worth a test with an actual basePath-configured app to confirm.
| @@ -511,14 +519,11 @@ function BrowserRoot({ | |||
| } | |||
There was a problem hiding this comment.
The effect dependency change from treeState.interceptionContext to treeState.previousNextUrl makes sense — the history state now persists previousNextUrl, not interceptionContext. But note that the renderId !== 0 guard means this effect only runs during the initial mount (renderId === 0). For subsequent navigations, the history state update happens in createNavigationCommitEffect instead. The guard and the dependency array are both correct.
| const destinationPath = finalUrl.pathname.replace(/\.rsc$/, "") + finalUrl.search; | ||
| replaceHistoryStateWithoutNotify( | ||
| createHistoryStateWithInterceptionContext(null, requestInterceptionContext), | ||
| createHistoryStateWithPreviousNextUrl(null, requestPreviousNextUrl), |
There was a problem hiding this comment.
On redirect, requestPreviousNextUrl is written to the intermediate history entry. Then on line 979 it's forwarded to the recursive navigate call as previousNextUrlOverride. This correctly preserves the original source-route provenance through redirect chains. Good.
| await expect(page.locator('[data-testid="photo-modal"]')).not.toBeVisible(); | ||
| }); | ||
|
|
||
| test("hard reload after intercepted navigation renders the full page", async ({ page }) => { |
There was a problem hiding this comment.
Good test — this directly validates the hard-load normalization at boot (createHistoryStateWithPreviousNextUrl(window.history.state, null)). Without that line in main(), the reload would carry the previousNextUrl from state and re-request the intercepted view.
Consider also adding a test for the back-then-forward case mentioned in the PR description ("back/forward restores intercepted modal view"). The test plan references it but it's not in this diff.
|
Review posted. Here's a summary of the feedback: Key findings:
|
Avoid calling getBrowserRouterState() twice in the refresh branch of getRequestState by binding previousNextUrl to a local. Behavior-preserving nit raised in PR review.
After previousNextUrl became the source of truth for interception context in history state, nothing in production writes the legacy __vinext_interceptionContext key anymore. The fall-through read in getCurrentInterceptionContext was vestigial and getCurrentInterceptionContext now derives strictly from the current pathname. - Removes the unused VINEXT_INTERCEPTION_CONTEXT_HISTORY_STATE_KEY export - Removes readHistoryStateInterceptionContext - Updates the prefetch-cache unit test to assert the new pathname-derived contract instead of the legacy history-state override
Adds the back-then-forward case for App Router intercepting routes: after intercepted navigation from /feed to /photos/42, going back to /feed and then forward should restore the modal (rather than rendering the full /photos/42 page). Directly exercises the previousNextUrl read in the traverse branch of getRequestState.
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat: track previousNextUrl for intercepted App Router entries
The design is solid. Separating previousNextUrl (full URL including search params, persisted in history state) from interceptionContext (basePath-stripped pathname, derived on demand) is clean layering that keeps each concern well-scoped. The hard-load normalization at boot, redirect chain forwarding, and server-action fallback via currentState.previousNextUrl are all handled correctly.
Prior review items resolved
The previous bonk review raised two main concerns that are now addressed:
- Stale
readHistoryStateInterceptionContextdead code — fully removed from imports and exports.getCurrentInterceptionContext()now derives purely fromwindow.location.pathname, which is correct. - Missing back/forward E2E test — present in the current diff at
advanced.spec.ts:153.
Remaining items
Stale "belongs to PR 5" comments (not in diff, so noting here): Lines 758 and 1097 of app-browser-entry.ts still say behavior "belongs to PR 5", but this is PR 5. The server action comment (line 758) should be reworded to say interception headers on action POSTs are intentionally deferred. The HMR comment (line 1097) should say interception preservation across HMR is intentionally deferred. Both are correct design decisions — the comments just reference a future that has already arrived.
getCurrentNextUrl() includes basePath: getCurrentNextUrl() returns the raw window.location.pathname + window.location.search, which includes basePath when configured. resolveInterceptionContextFromPreviousNextUrl correctly strips it. The round-trip works, but there's no test coverage for a basePath-configured app. Not a blocker, but worth adding in a follow-up.
Otherwise LGTM. The test coverage is thorough — hard reload, back/forward restoration, and router.refresh() all exercise the critical paths.
| case "refresh": { | ||
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | ||
| return { | ||
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | ||
| currentPreviousNextUrl, | ||
| __basePath, | ||
| ), | ||
| previousNextUrl: currentPreviousNextUrl, | ||
| }; |
There was a problem hiding this comment.
Nit: getBrowserRouterState() is called at line 338, then .previousNextUrl is accessed on the result at line 338 and again at line 344 (via re-evaluation). The ref read is cheap so this is correct, but binding once is cleaner:
| case "refresh": { | |
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | |
| return { | |
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | |
| currentPreviousNextUrl, | |
| __basePath, | |
| ), | |
| previousNextUrl: currentPreviousNextUrl, | |
| }; | |
| case "refresh": { | |
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | |
| return { | |
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | |
| currentPreviousNextUrl, | |
| __basePath, | |
| ), | |
| previousNextUrl: currentPreviousNextUrl, | |
| }; | |
| } |
| }): Promise<PendingNavigationCommit> { | ||
| const elements = await options.nextElements; | ||
| const metadata = readAppElementsMetadata(elements); | ||
| const previousNextUrl = options.previousNextUrl ?? options.currentState.previousNextUrl; |
There was a problem hiding this comment.
This fallback (options.previousNextUrl ?? options.currentState.previousNextUrl) is the key to server-action merges preserving interception state without explicit plumbing. commitSameUrlNavigatePayload omits previousNextUrl, so it naturally inherits from the committed state. Clean design.
| window.history.state, | ||
| treeState.interceptionContext, | ||
| ), | ||
| createHistoryStateWithPreviousNextUrl(window.history.state, treeState.previousNextUrl), |
There was a problem hiding this comment.
Minor observation: treeState.previousNextUrl is in the dependency array, but the renderId !== 0 guard at line 519 means this effect only executes on the initial mount. After that, createNavigationCommitEffect handles history state updates. The previousNextUrl dependency is effectively inert after mount — [treeState.renderId] alone would be sufficient. Not a blocker, just noting for clarity.
| replaceHistoryStateWithoutNotify( | ||
| createHistoryStateWithPreviousNextUrl(window.history.state, null), | ||
| "", | ||
| window.location.href, | ||
| ); |
There was a problem hiding this comment.
Correct and important: browsers preserve history.state across reloads, so without this normalization, a reload would carry over previousNextUrl from the intercepted navigation and the refresh request would re-fetch the intercepted view instead of the full page. This also implicitly cleans up any lingering __vinext_interceptionContext from old history entries via the cloneHistoryState + selective key write pattern.
|
Review posted successfully. Here's a summary of the feedback: Key findings:
|
Two inline comments in app-browser-entry.ts referenced "PR 5" as a future PR, but this is PR 5. Reworded to describe the deferred scope as-is: interception context on server-action POSTs and HMR re-renders is intentionally not propagated.
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat: track previousNextUrl for intercepted App Router entries
This is a well-structured PR. The core design — separating previousNextUrl (full URL with search params, persisted in history state) from interceptionContext (basePath-stripped pathname, derived on demand via resolveInterceptionContextFromPreviousNextUrl) — is clean and correct. Each concept has a single source of truth, and the derivation path is explicit.
The three critical interception behaviors are all handled correctly:
- Hard reload breaks out —
main()clearspreviousNextUrlfrom the active history entry at boot, so the refresh RSC request has no interception context. - Back/forward restores —
readHistoryStatePreviousNextUrlreads the persisted URL fromevent.state, andresolveInterceptionContextFromPreviousNextUrlderives the correct interception context for the RSC request. router.refresh()preserves — therefreshcase ingetRequestStatereadspreviousNextUrlfrom the committed React state (not history), so the RSC request re-fetches the intercepted view.
The redirect chain forwarding (requestPreviousNextUrl passed through to the recursive navigate call) is correctly handled. The server-action fallback via options.previousNextUrl ?? options.currentState.previousNextUrl in createPendingNavigationCommit elegantly preserves interception provenance without explicit plumbing.
Prior review concerns (stale readHistoryStateInterceptionContext dead code, missing back/forward E2E test) are fully resolved. The old __vinext_interceptionContext references have been completely removed from the codebase.
Test coverage is thorough: unit tests for the state/history helpers, and E2E tests for all three critical paths (hard reload, back/forward, router.refresh()).
One minor nit and one observation below. LGTM.
| case "refresh": { | ||
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | ||
| return { | ||
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | ||
| currentPreviousNextUrl, | ||
| __basePath, | ||
| ), | ||
| previousNextUrl: currentPreviousNextUrl, |
There was a problem hiding this comment.
Nit (carried over from prior review): getBrowserRouterState() is evaluated twice — once at line 338 and again implicitly at line 338 to access .previousNextUrl, then the same expression repeats at line 344. The ref read is cheap and correct, but binding once is slightly cleaner:
| case "refresh": { | |
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | |
| return { | |
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | |
| currentPreviousNextUrl, | |
| __basePath, | |
| ), | |
| previousNextUrl: currentPreviousNextUrl, | |
| case "refresh": { | |
| const currentPreviousNextUrl = getBrowserRouterState().previousNextUrl; | |
| return { | |
| interceptionContext: resolveInterceptionContextFromPreviousNextUrl( | |
| currentPreviousNextUrl, | |
| __basePath, | |
| ), | |
| previousNextUrl: currentPreviousNextUrl, | |
| }; | |
| } |
| window.location.href, | ||
| ); | ||
| }, [treeState.interceptionContext, treeState.renderId]); | ||
| }, [treeState.previousNextUrl, treeState.renderId]); |
There was a problem hiding this comment.
Minor observation: treeState.previousNextUrl is in the dependency array, but the renderId !== 0 guard at line 519 means this effect only executes on the initial mount (renderId starts at 0). After mount, createNavigationCommitEffect handles all history state updates. The previousNextUrl dep is effectively inert post-mount — [treeState.renderId] alone would suffice. Not a problem, just noting for future clarity.
|
Review posted and approved. Here's a summary: Verdict: Approved The PR is well-designed. The core architecture of separating All three critical interception behaviors work correctly:
Prior review concerns (dead
|
3 participants
Summary
Part of #726 (PR 5). This builds on PR 4's interception-aware payload IDs and cache keys by tracking
previousNextUrlin the App Router browser state. Intercepted navigations now remember the URL they came from, so refresh/back-forward behavior can distinguish soft interception from direct loads.The practical effect is that
/feed -> /photos/42modal navigations now behave correctly across the three important cases: browser reload breaks out to the full page, back/forward restores the intercepted modal view from history state, androuter.refresh()keeps the intercepted modal instead of silently switching to the direct page tree.What changed
app-browser-statenow carriespreviousNextUrlalongsideelements,interceptionContext,routeId, androotLayoutTreePath, with helpers for persisting and reading it from history state__vinext_previousNextUrlinstead of the old interception-context field, preserving the URL active before a soft navigation while keeping the existing scroll restoration state intactnavigaterecords the current URL aspreviousNextUrl,traversereconstructs interception context fromevent.state, andrefreshreconstructs it from committed router state so refresh preserves interception without changing hard-load behaviorpreviousNextUrlfrom the active history entry so full document reloads still break out of interception even though browsers preservehistory.stateacross reloadspreviousNextUrlforward so replacement navigations do not lose the source-route provenancepreviousNextUrlstate/history helpers plus E2E coverage for reload, back/forward restoration, androuter.refresh()on intercepted routes. The modal fixture now exposes a smallrouter.refresh()button to exercise the public APIapp-browser-entrycomments so they match the current dispatch/control-flow names after the refactorWhat this does NOT do
Server action interception-request parity is still intentionally deferred. This PR preserves the browser-side
previousNextUrlstate during server-action merges, but it does not add interception headers to the action POST itself.Test plan
vp test run tests/app-browser-entry.test.tsvp test run tests/app-router.test.tsvp run test:e2e tests/e2e/app-router/advanced.spec.ts --grep 'hard reload after intercepted navigation renders the full page|back then forward restores intercepted modal view|router.refresh preserves intercepted modal view'git commitpre-commit hook (vp check --fix)